Skip to content

[RF] ToyMCSampler improvements for binned toys #20171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[RF] ToyMCSampler improvements for binned toys #20171

wants to merge 3 commits into from
+51 −38

Conversation

@cburgard
Copy link
Contributor

This Pull request:

Adds the ability to control the ToyMCSampler to emit binned toys.
Makes use of this new capability in the HypoTestCalculator in cases where the observed data is binned.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #

@cburgard cburgard changed the title [RF] ToyMCSampler improvements for binned toys [RF] ToyMCSampler improvements for binned toys [WIP] Oct 22, 2025
@cburgard
Copy link
Contributor Author

This PR is still WIP. This is because the use case relies on #20097 being merged first (even though the functionalities are not closely related).

@guitargeek guitargeek marked this pull request as draft October 22, 2025 16:08
@guitargeek guitargeek self-assigned this Oct 22, 2025
@guitargeek
Copy link
Contributor

I have merged #20097. Could you please rebase this PR?

Also, could you please add a commit where you add both of you to the list of contributors for ROOT 6.38 (insert in alphabetical order)?
https://github.com/root-project/root/blob/master/README/ReleaseNotes/v638/index.md

Thanks a lot!

@github-actions
Copy link

github-actions bot commented Oct 22, 2025 •
edited
Loading

Test Results

    22 files      22 suites   3d 18h 55m 45s ⏱️
 3 696 tests  3 688 ✅ 0 💤   8 ❌
79 361 runs  79 185 ✅ 0 💤 176 ❌

For more details on these failures, see this check.

Results for commit b2f731f.

♻️ This comment has been updated with latest results.

@cburgard cburgard force-pushed the toymcsampler-bugfix branch from fbb3468 to c5dddab Compare October 23, 2025 09:19
@cburgard cburgard force-pushed the toymcsampler-bugfix branch from c5dddab to 6dbedd7 Compare October 23, 2025 09:23
@cburgard
Copy link
Contributor Author

I have merged #20097. Could you please rebase this PR?

Also, could you please add a commit where you add both of you to the list of contributors for ROOT 6.38 (insert in alphabetical order)? https://github.com/root-project/root/blob/master/README/ReleaseNotes/v638/index.md

Thanks a lot!

Thanks a lot!

Done and done. I've removed the WIP marker now.

@cburgard cburgard changed the title [RF] ToyMCSampler improvements for binned toys [WIP] [RF] ToyMCSampler improvements for binned toys Oct 23, 2025
@cburgard cburgard marked this pull request as ready for review October 23, 2025 09:25
@cburgard cburgard requested a review from dpiparo as a code owner October 23, 2025 09:25
guitargeek
guitargeek approved these changes Oct 26, 2025
View reviewed changes
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much! This change makes sense to me.

guitargeek
guitargeek reviewed Oct 27, 2025
View reviewed changes
roofit/roostats/src/HypoTestCalculatorGeneric.cxx
Comment on lines +67 to +68
// --- Ensure the ToyMCSampler generates toys with the same structure as the observed data
toymcs->SetProtoData(&data);
Copy link
Contributor

@guitargeek guitargeek Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change in behavior that likely causes the RooStats tests to fail. I'll take a look.

guitargeek
guitargeek requested changes Oct 27, 2025
View reviewed changes
Copy link
Contributor

@guitargeek guitargeek left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR actually introduces a behavioral change that causes the RooStats tests like stressRooStats to fail. Even if the warning about duplicate args is silenced in this line by doing allVars.add(*prototype->get(), true).

@cburgard, did you intend the behavior change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guitargeek guitargeek guitargeek requested changes

@lmoneta lmoneta Awaiting requested review from lmoneta lmoneta is a code owner

@dpiparo dpiparo Awaiting requested review from dpiparo dpiparo is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@guitargeek guitargeek

Labels

in:RooFit/RooStats

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cburgard @guitargeek